[RNMobile] Fix UBE Text Selection Bug on Android #34668
Conversation
With this commit, a new JS file is loaded in 'GutenbergWebViewActivity.java'. This new file will have our custom JS, which will load when the UBE is open.
An event listener is added as part of this commit, which will listen for changes in focus in the UBE. In addition, an if statement that checks for the specific elements we're interested in (the toolbars dropdown menus) has been added. This if statement will be fleshed out in future commits.
The text selection is reset by utilising JavaScript's setBaseAndExtent' method. This has the effect of removing the text selection toolbar when a dropdown menu is pressed, while maintaining the original selection. See here for more details on setBaseAndExtent: https://developer.mozilla.org/en-US/docs/Web/API/Selection/setBaseAndExtent
SiobhyB
changed the title
|
Size Change: +2.16 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
SiobhyB
changed the title
This PR renames the 'custom-js' file to 'editor-behavior-overrides.js' in an attempt to clarify its purpose.
As this is a somewhat hacky and unintuitive fix, this commit adds a clarifying comment along with a link back to the PR for more details.
SiobhyB
added
Mobile App - i.e. Android or iOS
The if/else statement was only checking whether specific buttons were active before resetting the text selection. With this commit, an extra check for whether text is actually selected has been added. The code for resetting the selection will therefore only be run when necessary (i.e. it's not necessary if there's no selection).
With this commit, changes are made to access the 'activeElement' via 'document', rather than 'window'. Accessing directly via 'window' was not working as expected. It was necessary to still reference 'window' to bypass lint errors.
This was accidentally removed with the last commit.
|
@fluiddot, would you maybe have the time to review this PR this week? I've attempted to outline the reasoning behind the changes as clearly as possible, but it's also a pretty hacky fix and I'd be happy to clarify anything (via Zoom or text) if needed. I've also generated an installable build that's available at wordpress-mobile/WordPress-Android#15304. |
Sure, I'll spare some time this week to review it 👍, I'll also assign myself as a reviewer. |
Previously the selected text was reset on the JavaScript side of the codebase (using the 'setBaseAndExtent' method). This commit refactors the code so that the Android side of the codebase is notified instead, via the newly created 'hideTextSelectionContextMenu' method. The aim here is to make use of Android's built-in functions around ActionMode, in order to make the code and functionality more stable.
This commit taps into Android's ActionMode to hide the text selection toolbar whenever the 'hideTextSelectionContextMenu' function is called. More specficially, it uses the 'finish' method: https://developer.android.com/reference/android/view/ActionMode#finish()
I accidentally ommitted the 'mActionMode' variable from previous commits.
|
@fluiddot, I've refactored this PR, following our Zoom, to utilise Android's ActionMode and the installable build can be found here. I initially tried using the UBE.hide.recording.movThis limitation of
Let me know if you think there's still a way that we could prevent the need for two taps here, I'd love to get it working without that quirk, if possible. 🙇♀️ If it's not possible, I think two taps would still be a better experience than the current blocking behaviour. |
# Conflicts: # packages/react-native-editor/CHANGELOG.md
Thanks for trying the approach with the
I explored the idea you had about automatically clicking the button when hiding the floating bar and I implemented it in this commit. So far looks like addresses the issue and doesn't require the double-tap 🎊 . It would be great if we test it further and if it goes well, I think we could wrap up the PR and merge it. |
|
@fluiddot, thanks so much for this 🙇♀️ I created a new installable build here and tested it on a physical Pixel 5. It worked well, with a slight flicker, which we discussed isn't avoidable with this approach. I didn't find it jarring, though, and think it seems like a good solution. |
|
@SiobhyB I've implemented a new approach because I noticed that for some of the dropdown menus the last solution I implemented wasn't hiding the floating menu. For the new solution, I refactored a bit the code and now instead of listening to focus events, we listen to clicks. This way we can prevent the propagation of the event and delay the click right after the context menu is hidden, which prevents flickering. Besides, I added listeners to know when the context menu is visible and only override the click event if it's being shown. I've tested further and I think this could be definitive solution for this 🤞 , nevertheless, I'd appreciate it if we could test it again before approving it. Let me know your opinion regarding this approach and if you'd like to continue with it. Otherwise, I'll be more than happy to revert it and go with the previous approach 👍 . |
|
@fluiddot, sorry I didn't get back to you yesterday! I wanted to make sure I took the time to understand the changes that have been implemented. I've done that now and it all makes sense to me, I also created another installable build and confirmed all works super smoothly on my Pixel 5 🎉 To tie things up, I created the issue for the mobile web at #35447, as we discussed. I'm happy to ship this if you are now, too :D |
No worries, I'm happy to see that works smoothly 😊. If you have any doubt about the changes I made please let me know, I'll be more than happy to jump into a quick call to explain them.
Awesome, thanks for creating the issue 🙇 .
Yeah, I'm going to do a final review as double-check but I think we already tested enough to merge it. |
|
Thanks so much for all your support on this one, @fluiddot 🙇♀️ I'll go ahead with the merge domino! :D |
Fixes: wordpress-mobile/gutenberg-mobile#3862
Description
As outlined in more detail at wordpress-mobile/gutenberg-mobile#3862, the floating toolbar that appears when text is selected blocks access to dropdown menu items in some cases. This is specific to the Unsupported Block Editor (UBE) and can prevent users from being able to format their text selection.
With this PR, the text selection toolbar is now correctly dismissed when a "parent" menu item is tapped in the UBE's main toolbar, so users will always be able to access the dropdown menu items.
How has this been tested?
Screenshots
You'll see that the text selection toolbar is now being correctly dismissed, while maintaining the selection.
Screen.Recording.2021-09-20.at.12.04.54.mov
Types of changes
This PR introduces a bug fix (a non-breaking change which fixes an issue), with the following high-level notes on the code changes:
Initial solution, before refactor:
Updated, current solution:
hideTextSelectionContextMenuis used to notify the Android side of the codebase when buttons impacted by the bug are tapped.hideTextSelectionContextMenufunction is called. More specficially, ActionMode'sfinishmethod is utilised to remove the toolbar.Checklist:
*.native.jsfiles for terms that need renaming or removal).